sequenceDiagram
  participant LoginForm
  participant AuthStore
  participant TestLoginEndpoint
  participant AuthApi
  LoginForm->>AuthStore: devLogin(email)
  AuthStore->>TestLoginEndpoint: POST /test/login
  TestLoginEndpoint-->>AuthStore: token and optional identity data
  AuthStore->>AuthStore: setAccessToken(token)
  LoginForm->>AuthApi: getMe()
  AuthApi-->>LoginForm: user identity

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Review follow-ups applied — 574f96c

All four findings addressed. npm run typecheck / npm test (90 tests, 12 files) / npm run build / npx publint all green locally — the exact four steps .github/workflows/ci.yml runs.

Finding 1 (P2) — formatting splice — FIXED

handleResend's opening brace and first statement were collapsed onto one line at login-form.tsx:291. Restored.

On a format gate: this repo has no formatter at all — no prettier, no biome, no .prettierrc/biome.json, and nothing in devDependencies. That is why CI never caught it. Suggested follow-up ticket (deliberately not done here, since it would reformat files this PR does not touch): add prettier + a format:check CI step. Related doc drift worth folding into the same ticket — AGENTS.md has claimed make build # clean + lint + typecheck + compile (PREFERRED pre-publish gate) since before this branch, but there is no Makefile in this repo and there is no lint script. The real gate is the four npm scripts above.

Finding 2 — uncommitted .reposkein/.gitignore — COMPLETED PROPERLY

The ignore rules alone were inert: both files were tracked on origin/main, and d801638 re-committed 128 lines of regenerated churn. Kept the .gitignore change and ran git rm --cached .reposkein/nodes.jsonl .reposkein/edges.jsonl, matching the workspace convention that the derived graph is not tracked.

Checked before untracking: zero of the 143 nodes carry a summary, so no authored content is lost — only the deterministic, regenerable graph. Verified afterwards that git status is clean, both files still exist on disk, and git check-ignore -v resolves them to the new rules.

One deliberate deferral: .gitattributes still declares merge=reposkein-jsonl for the two now-untracked paths. Those declarations are inert for untracked files, and removing the file was out of scope for this fixup — flagging it rather than silently leaving it unexplained.

Finding 3 (P2) — dev portal guard swallowed getMe failure — FIXED

Confirmed real. handleDevLogin resolved the user type through a separate authApi.getMe(), swallowed the failure to null, then only rejected on if (authenticatedUserType && authenticatedUserType !== userType). A transient /auth/me failure therefore let an importer session stand inside the producer portal. verifyOtp cannot have this hole because it returns result.user inline.

An unresolvable user type is now a failed check, not a pass:

if (!authenticatedUserType) {
  const msg = "Couldn't verify your account type. Try again.";
  authStore.clearAccessToken();
  setDevError(msg);
  onError?.({ code: "DEV_LOGIN_USER_TYPE_UNVERIFIED", message: msg });
  return;
}

Two tests added, covering both ways the type can come back unresolvable: getMe throws, and getMe resolves without a userType. Both assert clearAccessToken was called and onLoginSuccess was not.

Sabotage-checked, not assumed: reverting the guard to the old if (authenticatedUserType && ...) form fails 2 of 11 tests in login-form-dev-bypass.test.tsx. The tests are not inert.

Left alone on purpose: the token is adopted before the portal guard runs — but handleOtpSubmit does exactly the same thing. That is pre-existing parity, not a regression introduced here. I documented it at the call site so nobody later "fixes" only the dev path and leaves the OTP path holding the same shape.

Finding 4 (P3) — tree-shaking asserted nowhere — CLOSED, with one correction

Closed with a real build-output assertion: __tests__/dev-bypass-treeshake.test.ts bundles login-form.tsx through esbuild the way a consumer's Vite production build does (import.meta.env.DEV statically defined, tree-shaking on) and greps the emitted code. It builds both directions — PROD must not contain the dev markup, DEV must — so the assertion cannot go inert if the sentinels drift. esbuild was already present as vitest's own transform dependency; it is now declared explicitly in devDependencies rather than relied on transitively.

Correction to the suggested approach: DEV_LOGIN_EMAIL_STORAGE_KEY is not a valid sentinel. I checked the actual bundler output before writing the test — the literal "cellarnode.dev.login-email" survives into production bundles, minified or not:

// src/react/dev-sign-in.tsx   (present in the PROD bundle)
var DEV_LOGIN_EMAIL_STORAGE_KEY = "cellarnode.dev.login-email";
function readDevLoginEmail() { ... }
function rememberDevLoginEmail(email) { ... }

Only the DevSignInBypass component is statically eliminated. The two storage helpers are called from live function bodies behind runtime if guards, so no bundler can drop them — they ship as unreachable code. A few bytes and no behaviour (both are no-ops unless called), but it means the branch's docs overclaimed. AGENTS.md, CHANGELOG.md, and the three source comments said Rollup "drops src/react/dev-sign-in.tsx with it"; corrected to state precisely what is and is not eliminated. The test's sentinels are the component's own copy ("Dev sign-in (skip the code)", "Development only"), which genuinely are absent in PROD.

Sabotage-checked in both shapes the finding worried about:

• Removing the import.meta.env.DEV && guard from the JSX → test fails.
• The exact refactor named in the finding — reading the flag through an indirection the bundler cannot fold statically → test fails.

So the coverage gap the finding described is now genuinely closed, not nominally.

Anti-enumeration contract

Untouched. The uniform-404 copy still names only ENABLE_TEST_ENDPOINTS=true and never the address, and the test pinning it ("surfaces the gate-off hint when the backend 404s, without blaming the address", including its not.toMatch(/no account|does not exist|not found/i) assertion) still passes.

One thing to be aware of about this branch's history

A parallel session committed and pushed a34f347 from this worktree while this work was mid-flight. It swept up my in-progress edits at an instant when login-form.tsx was in a deliberately sabotaged state — that pushed commit contains the fail-open guard, so CI on a34f347 fails 2 tests. 574f96c restores the fail-closed version, and the branch tip is green. Nothing was lost, but review 574f96c as the authoritative state of the guard, not a34f347. I did not rewrite the pushed commit, since another session is active in this checkout.

Branch-history note (no action needed, but read before merging): the middle commit a34f347 on this branch has RED CI. It was committed and pushed by a parallel session at 07:23 while an agent had login-form.tsx in a deliberately-sabotaged state for mutation testing, so it captured a fail-OPEN portal guard. The tip commit 574f96c restores fail-closed and the branch tip is green (typecheck / 90 tests / build / publint all pass).

This PR is squash-merged, so the intermediate state never reaches main — the merge takes the tip's diff. Do NOT rebase or cherry-pick individual commits from this branch expecting them to be independently sound.

Related: a parallel session is live in this same checkout, which is how the interleave happened.

Bot review dispositions — CEL-1364

Fixup pushed as 5a8e235. Gates: npm run typecheck, npm test (93 passed, was 90), npm run build, npx publint — all four green.

Items 8-11 were filed against a34f347, an intermediate commit pushed while the file was deliberately sabotaged for mutation testing. All four verified resolved at tip; no code was touched for them. For #10 I additionally checked that the JSDoc and the inline comment describe the fail-closed behaviour ("fails CLOSED: an unresolvable user type clears the token instead of standing" / "an unresolvable answer is a FAILED check, never a pass") — they do, so nothing to change.

1. Inert test

The test seeded the storage key with the same address it then asserted, so it passed whether or not rememberDevLoginEmail ever ran. It now seeds a stale address, lets the DEV prefill load it, types a different one, and asserts the typed address is what devLogin received and what ended up in storage. Seed and expectation now disagree, so only the code under test can reconcile them.

Non-inertness verified by mutation — with the rememberDevLoginEmail(normalizedEmail) call removed:

× signs in as the address in the form and remembers THAT address
  Tests  1 failed | 12 passed (13)

Same check run for the other three new tests; each mutation kills exactly its target.

2. Missing catch

devLogin is optional on AuthStore, so nothing forces an implementation to resolve a DevLoginFailure rather than reject — and clearAccessToken / onLoginSuccess can throw independently. From a click handler that surfaced as an unhandled rejection: finally still re-enabled the button, so the developer saw a control that silently did nothing. Failures now go through the same setDevError + onError channel as every other exit, with code DEV_LOGIN_UNEXPECTED. New test: a store whose devLogin rejects → alert rendered, onError called, onLoginSuccess not called, button re-enabled.

3. The race

disabled={isSubmitting || !normalizedEmail} on the OTP "Continue" button ignored isDevSubmitting, so a developer could request an OTP mid-devLogin, advance to the OTP step, and have the resolving bypass call onLoginSuccess() from a step that no longer rendered it. isDevSubmitting now participates, making the guard reciprocal, which is what DevSignInBypassProps.disabled claimed all along. New test holds devLogin pending and asserts Continue is disabled and requestOtp is never called.

4. null JSON body

res.json() resolving is not the same as "we got an object" — a body of literal null parses fine, and extractAccessToken dereferences its argument, so it threw straight out of a function whose result type promises it never does. Guarded before extraction; arrays fall through to the same malformed result. New test asserts a resolved failure, not a rejection.

5. Barrel exports

DevSignInBypass, DEV_LOGIN_EMAIL_STORAGE_KEY, readDevLoginEmail and rememberDevLoginEmail are gone from src/react/index.ts. The gate is the single import.meta.env.DEV call site inside LoginForm; an exported symbol carries no gate, so a consumer could render the bypass UI or persist an address to localStorage from a production build. No consumer in the workspace imported them, and the tests already imported ../src/react/dev-sign-in.js directly, so no test was weakened. dev-bypass-treeshake.test.ts is untouched and still asserts both directions (DEV bundle contains the markers, prod bundle does not).

6. devLogin in production bundles — declined, with reasoning

I verified the backend claim rather than assuming it. apps/cellarnode/src/features/test-only/test-routes.ts gates on isTestEndpointsEnabled() = !isProdLike() && process.env.ENABLE_TEST_ENDPOINTS === "true", where isProdLike() covers both NODE_ENV=production and MODE=prod. That predicate runs twice: at mount (public.ts:265, so the routes are never registered) and inside every handler. /test/login additionally returns a uniform 404 for "gate off" and "no such user". In production the route does not exist, so a shipped devLogin can only ever resolve { ok: false, reason: "test-endpoints-disabled" }.

I declined the runtime gate for two reasons:

1. It would eliminate nothing. The check would sit in devLogin's own live body — the same position that, as our tree-shaking test already documents, keeps readDevLoginEmail / rememberDevLoginEmail in production bundles. "It tree-shakes away" is not true for this surface and I am not going to write a comment claiming it is.
2. It would break the core's portability. auth-store.ts is the framework-agnostic entry and has zero import.meta references today; the ambient import-meta-env.d.ts exists for the React subpath. Under plain Node ESM import.meta.env is undefined, so import.meta.env.DEV throws a TypeError — reintroducing, in the same method, exactly the never-throws violation item 4 just fixed. Writing import.meta.env?.DEV avoids the throw but defeats Vite's static replacement, so it buys nothing either.

Against that, the residual exposure is one bundled string naming ENABLE_TEST_ENDPOINTS. That flag is already documented in this repo's README and AGENTS.md and in the backend repo, and an attacker does not need our bundle to POST /test/login — the endpoint is reachable or not purely as a function of server config. So I judged this defense-in-depth with negative net value, and left the copy as-is.

What I did instead was make the documentation state the actual behaviour. README.md now splits dropped (the DevSignInBypass component and its module) from kept (devLogin, its failure copy, the two storage helpers), AGENTS.md carries the same split as a rule that must not drift plus the declined-gate rationale, and the AuthStore.devLogin JSDoc and the DEV_LOGIN_MESSAGES comment say plainly that they ship and why that is safe.

There is a clean way to eliminate the copy if we ever want it: move DEV_LOGIN_MESSAGES into the DEV-only React module and have the UI map reason → text. That drops message from DevLoginFailure, which is a public-API change and not a review fixup — noting it rather than doing it here.

7. Docs

AGENTS.md and README.md export lists now match the real surface and call out the deliberate non-exports. AGENTS.md's make build gate was stale (there is no Makefile in this repo) and is replaced by the four actual CI steps. The CHANGELOG.md 0.14.0 entry no longer claims the dev symbols are exported.

Anti-enumeration

Untouched and still pinned. The uniform-404 copy never reveals whether an address exists, devLogin() still maps that 404 to the single test-endpoints-disabled reason, and the test asserting the alert contains ENABLE_TEST_ENDPOINTS=true while matching none of /no account|does not exist|not found/i is green.